scripts: harden rmg_kinetics & rmg_thermo#889
Open
alongd wants to merge 1 commit into
Open
Conversation
Fixes from CC review: deep-copy DB kinetics before scaling so a second query can't read a doubly-scaled A, gate change_rate on Arrhenius/EP so non-Arrhenius forms (Chebyshev/PLOG/etc.) are skipped safely, add sys.path bootstrap so `from common import ...` works regardless of CWD, add an --output flag so the input YAML isn't overwritten, document the training-entry filter and output units, and harden the helper's debug print against reactions without reactants. Adds argparse + subprocess unit tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alongd
changed the title
There was a problem hiding this comment.
Pull request overview
This PR hardens ARC’s standalone RMG helper scripts (thermo + kinetics) based on prior review feedback, improving correctness across repeated database queries, safer handling of unsupported kinetics forms, and more robust CLI invocation/testing.
Changes:
- Added
--output/-osupport (and preserved default overwrite behavior) so scripts can write results to a separate YAML file. - Hardened
rmg_kinetics.pyby deep-copying database kinetics before in-place mutation and guardingchange_rate()to Arrhenius-like kinetics. - Improved script portability by bootstrapping
sys.pathsofrom common import ...works regardless of current working directory; added/expanded unit tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
arc/scripts/rmg_thermo.py |
Adds sys.path bootstrap and supports writing results to an optional output YAML path. |
arc/scripts/rmg_kinetics.py |
Deep-copies kinetics before mutation, guards degeneracy scaling, improves debug printing, adds --output, and updates documentation. |
arc/scripts/common.py |
Extends the shared argparse parser with an optional --output/-o flag. |
arc/scripts_test.py |
Adds unit tests for argparse, helper behavior, and --output non-overwrite behavior via conda run. |
Comments suppressed due to low confidence (1)
arc/scripts/rmg_kinetics.py:117
- A_units is only set for bimolecular vs 'everything else'. For termolecular reactions (len(reactants)==3), this will incorrectly treat A as s^-1 and skip the needed m^6→cm^6 scaling, even though loop_families supports 3 reactants/products. Consider adding an explicit 3-reactant branch (and corresponding scaling) or rejecting/flagging unsupported reaction orders.
# Families:
A_units = "cm^3/(mol*s)" if len(reaction.reactants) == 2 else "s^-1"
fam_list = loop_families(rmgdb, reaction)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+8
to
+10
| - ``A``: cm^3/(mol*s) for bimolecular reactions, s^-1 for unimolecular | ||
| (3-body: cm^6/(mol^2*s)). Reported in the units stored on the | ||
| Arrhenius object after the SI->cm conversion below. |
| Unit tests for ``rmg_kinetics.py`` helpers that don't need a full RMG database load. | ||
|
|
||
| Each test runs a tiny ``python -c`` snippet inside ``rmg_env`` so we can import | ||
| rmgpy and the script module directly. Stdout is parsed as YAML. |
Comment on lines
+207
to
+226
| """The new isinstance gate must skip ``change_rate`` for non-Arrhenius kinetics | ||
| (e.g. Chebyshev) rather than blindly mutating them.""" | ||
| snippet = textwrap.dedent(f""" | ||
| import sys | ||
| sys.path.insert(0, {self.SCRIPT_DIR!r}) | ||
| from rmgpy.kinetics import Arrhenius, ArrheniusEP | ||
| # Sanity: the script imports the same classes we test against. | ||
| import rmg_kinetics as rk | ||
| assert rk.Arrhenius is Arrhenius | ||
| assert rk.ArrheniusEP is ArrheniusEP | ||
| # The guard logic itself is a one-line isinstance check; replicate it here | ||
| # so a regression that drops the guard would fail the test. | ||
| from rmgpy.kinetics import Chebyshev | ||
| cheb = Chebyshev(coeffs=[[1.0, 0.0], [0.0, 0.0]], | ||
| kunits='cm^3/(mol*s)', | ||
| Tmin=(300.0, 'K'), Tmax=(2000.0, 'K'), | ||
| Pmin=(0.01, 'bar'), Pmax=(100.0, 'bar')) | ||
| assert not isinstance(cheb, (rk.Arrhenius, rk.ArrheniusEP)) | ||
| arr = Arrhenius(A=(1.0, 's^-1'), n=0.0, Ea=(0.0, 'J/mol')) | ||
| assert isinstance(arr, (rk.Arrhenius, rk.ArrheniusEP)) |
| ) | ||
| self.assertEqual(result.returncode, 0, f'thermo script failed: {result.stderr}') | ||
|
|
||
| # Input must be byte-identical (no overwrite). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deep-copy DB kinetics before scaling so a second query can't read a doubly-scaled A, gate change_rate on Arrhenius/EP so non-Arrhenius forms (Chebyshev/PLOG/etc.) are skipped safely, add sys.path bootstrap so
from common import ...works regardless of CWD, add an --output flag so the input YAML isn't overwritten, document the training-entry filter and output units, and harden the helper's debug print against reactions without reactants. Adds argparse + subprocess unit tests.